Conversation
| 8. Permissions: Folder Read Permission and dataset read permissions | ||
| 9. Metrics: Report and filter usage. |
There was a problem hiding this comment.
Are these requirements or non-requiremnents? I'm unclear on the break between them and the first seven.
| - Shows read-only summary: "Viewing {count} animal(s): {subject1}, {subject2}, ..." | ||
| - Shows "Modify Search" button that switches to ID Search mode with current subjects pre-populated | ||
| - Reports filter by URL subjects without requiring ID resolution | ||
| - No ID limit applies (URL-provided subjects are assumed already validated/resolved) |
There was a problem hiding this comment.
If it's worth capping the limit in "normal" usage (for performance, I guess?), it seems like we shouldn't exempt URL-provided values. But I'm also not really clear on why this is a useful mode.
There was a problem hiding this comment.
This is the same UI as participant history which completely derives the filters from the URL. This is pretty much only used when clicking on links generated in the EHR, so we assume we don't need to validate the parameters. In the normal search we're just trying to scope the amount of Ids that can be filtered, but may need to revisit this as we get more testing done.
| }, [activeReport]); | ||
|
|
||
| const handleFilterChange = useCallback(( | ||
| newFilterType: 'idSearch' | 'all' | 'aliveAtCenter' | 'urlParams', |
There was a problem hiding this comment.
Seems like a lot of separate hard-codings of these values.
There was a problem hiding this comment.
These were moved to variables.
| 2. Likelihood: Medium | ||
| 3. Mitigation: | ||
| 1. Id resolution feedback for in UI. | ||
| 2. Implemented as experimental feature to be able to view old and new animal history results side-by-side. |
There was a problem hiding this comment.
What does this feature flag control? Being able to open the new and old versions without toggling an experimental flag seems like it would be more convenient. Or does the flag control which is the target URL by default for existing UI?
There was a problem hiding this comment.
I updated the spec. This is going to just be hidden with link in admin page for now.
| - Repeat with tab-separated and semicolon-separated lists | ||
|
|
||
| 4. **Multi-Animal Search (Mixed Separators)** | ||
| - Enter IDs using multiple separators in single input: "ID1, ID2\nID3;ID4\tID5" |
There was a problem hiding this comment.
Because IDs can contain special characters the test should try IDs with separator values (e.g. ID123,A).
| - Verify validation error appears: "Maximum of 100 animal IDs allowed. You entered 101 IDs." | ||
| - Verify "Update Report" button is disabled | ||
| - Remove one ID to get back to 100 | ||
| - Verify error clears and "Update Report" button re-enables |
There was a problem hiding this comment.
Are IDs stored as string values? Would another boundary case be an ID that is a very long string, or MAXINT + 1?
| 4. **Mixed Valid/Invalid IDs:** Resolve valid IDs; show invalid in "Not Found" section | ||
| 5. **All IDs Not Found:** Display "Not Found" section only; reports panel shows no data message | ||
| 6. **Alias Resolves to Same ID:** If multiple input values resolve to the same animal ID, show all in "Resolved" section but pass de-duplicated list to reports | ||
| 7. **Special Characters in IDs:** Support IDs with hyphens, underscores, and other special characters |
There was a problem hiding this comment.
It might be worthwhile to call out some explicit special characters to test. For example, any characters that could be interpreted as a comment(// ' #), string(" ') or something could have special meaning in a SQL query or a url (? @ :).
Also worthwhile to consider where in the ID the special character is. An ID that starts with @123, or '123 or an ID that looks like a link / email address (123@abc.def)
| - Close browser, reopen bookmark | ||
| - Verify exactly 50 animals display correctly | ||
| - Verify no ID limit validation (URL Params mode bypasses 100 limit) | ||
| - Verify URL hash length doesn't cause browser issues |
There was a problem hiding this comment.
Another boundary case could be 10 IDs that are 1K characters long (or some other ridiculous size) and bookmark that.
| private void clickFilterButton(String buttonText) | ||
| { | ||
| clickButton(buttonText); // "All Records", "Alive at Center", or "ID Search" | ||
| sleep(500); // Allow mode transition |
There was a problem hiding this comment.
I'm not opposed to using sleeps, if they work that's great. Sometimes waiting for an element to go stale, the button or some element to show up can make the test a little more reliable and have a quicker run time. Something like:
shortWait().until(ExpectedConditions.stalenessOf(button));
Not sure if that will be applicable, but just a thought.
There was a problem hiding this comment.
I need to work through a few of these.
labkey-tchad
left a comment
labkey-tchad
left a comment
There was a problem hiding this comment.
Looks like this change is causing some test failures. ONPRC tests are failing folder import.
VALIDATION ERROR: Query study.aliasIdMatches failed validation!
org.labkey.api.query.QueryNotFoundException: Error on line 8: Query or table not found: study.alias
labkey-alan
left a comment
labkey-alan
left a comment
There was a problem hiding this comment.
I've left a lot of feedback. But in general we need to ensure that:
- We are properly memoizing things (especially functions used as callbacks)
- Our tests are actually testing things. A lot of the tests in this PR are not actually testing anything. I reviewed most tests, but not all, I suspect the tests I did not review are suffering from the same problems as the ones I did review.
- Interfaces should not be defined inline
- Prop types should be correct. Props that are always passed should be considered required, even if sometimes they're undefined. If something is sometimes undefined, say that in the type definition. If something is always defined, then we shouldn't treat is as sometimes not being defined (there's a lot of code that looks like
foo?.bar?.baz.()). Sometimes it's better to not render a component until all of the necessary props are defined, then you don't need any defensive code in the component, and there are less edge cases to handle.
We also need to rethink the component structure of the ReportTab and related components.
I also think that we shouldn't retain backwards compatibility with the hash URL, it adds a lot of code that we have to maintain, for very little benefit. We should use URL search params as they are intended to be used, not invent our own thing.
I also personally find this UI confusing. It doesn't make a lot of sense to have a text area with three buttons below it, but two of the buttons ignore the textarea. It seems to me like there should be something like a dropdown, or a radio group, where you select the filter mode. We should only render the textarea if the user selected "Search By Ids".
There was a problem hiding this comment.
I don't think there is any value in having a models folder with just an index.ts file with models defined. I would either:
- Get granular and split these model definitions up as needed into separate files in this folder
- Alternatively, get rid of the
modelsdirectory, instead move this file up one directory and rename it tomodels.ts
There was a problem hiding this comment.
Yeah since these are shared across files I just wanted them in a common place. I foresee more models being added in future stories and breaking up this file into multiple files. That's why I have it in a separate folder.
labkey-ui-ehr/src/ParticipantHistory/TabbedReportPanel/JSReportWrapper.tsx
Outdated
Show resolved
Hide resolved
| const { resolved, notFound } = resolutionResult; | ||
|
|
||
| // Separate direct matches from alias matches | ||
| const directMatches = resolved.filter(r => r.resolvedBy === 'direct'); |
There was a problem hiding this comment.
These variables should be memoized, you're creating a new array object every render cycle when filter is called, even if the resolved variable hasn't changed.
There was a problem hiding this comment.
I think memoizing this would actually raise the overhead. This is a pretty small calculation.
Also it's a pretty narrow use case (if at all) that this component would re-render with the same prop values (which would be the dependency on the useMemo). Most likely resolutionResult will have changed and these values will need to be recalculated any way.
There was a problem hiding this comment.
This component re-renders, and these values are re-computed, on every keystroke in the textarea, I have manually verified this. So it is actually called with the same value repeatedly, which is why I had suggested to use useMemo here. You could also use memo on the component itself, which should have the same effect as useMemo, since the incoming props would be compared against their previous values and only render when they change.
There was a problem hiding this comment.
Made component a memo
There was a problem hiding this comment.
I'm not seeing that, you may need to push, but that is probably the best solution.
Thanks for the great review @labkey-alan. This is a large PR, I appreciate the time on it. Most of your feedback has been incorporated. I commented on anything that wasn't. For your specific points.
I commented on the structure of ReportTab and related components below. I think given the integration with JS and ExtJS outside the react lifecycle and the setup required to render the non-react reports, this is a clean approach. Some of the components are not typical react components, but it's not a typical react interface as it extends outside the normal lifecycle. The patterns used, render-props and headless components, are all established patterns in react. On the URL I think you're right. We probably just need to break compatibility. That may be a story all on it's own to ensure we get it all correct and all links in EHR and otherwise updated. Thanks for the feedback on the UI. I will look into making that more clear in the next story. |
|
I have a bunch of pending review comments, but I wanted to leave a comment at the top level here because it's less likely to get lost in the noise of everything else.
Here is the math I am doing in my head to determine if we should
Because the performance hit from the overhead is effectively never disruptive to us you are, generally speaking, better off just memoizing the thing. If you are in the habit of doing that, then the likelihood that you'll introduce a performance issue is low. It also saves you the cognitive burden of constantly having to think to yourself "is this case one that could get out of hand and cause issues down the road?". You can absolutely write your code such that you only ever memoize stuff when you know for a fact that you'll get a benefit that outweighs the (most definitely negligible) overhead cost of memoizing, but in order to write that code you'll need to manually verify every component you write, and I don't really think that is worth the effort, when the cost is so low. You also need to ensure that when you're verifying your code you're doing it with realistic data, which is definitely easier said than done. An example of this: we had a performance issue in the Domain Designer, but it wasn't noticeable until you added something like 15-20 fields to a domain. Once you broke that threshold typing in any input on a Domain Field was noticeably slow. We didn't know about this issue until someone had a local domain with enough fields to reproduce the problem. |
Yeah I understand where you're coming from on that. I added it as a topic in our next front end meeting so we're consistent in that approach. It's a good time to formalize some of these standards across the team. |
There was a problem hiding this comment.
The code looks better, especially the new useReportTab hook which I think accomplishes a clearer component hierarchy, and is more idiomatic. I left a bunch of specific feedback, but I have some general feedback as well:
- There are a ton of comments in the code. In general I am a fan of code comments, and think that LabKey as a whole (myself included) do a poor job of commenting our code. That said, I think you should consider taking a pass to look at all of the comments across the files in this PR and trim the very obvious ones, and especially trim the ones that say something like "extracted from ", I don't think those are useful at all. I commented on a few of these, but there are a lot more that I didn't comment on.
- There are still a lot of tests that setup what seems like valid scenarios, but then fail to assert anything useful. If we cannot properly assert something, then I struggle to believe the test is useful.
Edit: some of my comments are on outdated code because you pushed changes while I was reviewing, but GitHub then hid those comments from me when it came to review submission time, so I couldn't remove them. I suspect some of those comments are already completely irrelevant.
| notFound: [], | ||
| }; | ||
|
|
||
| render(<IdResolutionFeedback isVisible={true} resolutionResult={resolutionResult} />); |
There was a problem hiding this comment.
Need to update these tests to no longer pass the flag now that it's been removed.
There was a problem hiding this comment.
This is interesting to me that this was not caught any where (other than IDE red squiggly). I've removed them but looking for better ways to catch this.
There was a problem hiding this comment.
Yeah this is just a problem with Typescript jest tests. This happens in all of our packages. Basically, because we're not using the webpack build when running tests, and we have our webpack build set to ignore tests, the type checks never happen on the jest files. It's mostly not a problem, because typically if you're altering the type you're also probably breaking the test in some way. We can maybe configure ts-jest to actually verify the types, but I haven't looked into that.
There was a problem hiding this comment.
I added a new npm target to test this. Can discuss in front end meeting.
There was a problem hiding this comment.
Yes and the way to do that within a react app is generally to use a ref and check if ref.current is not undefined. I'm sure this works, it's just not what I would call idiomatic react code.
| @@ -43,4 +43,8 @@ export const QueryReportWrapper: FC<{ report: ReportConfig; tab: any }> = memo(( | |||
| }, [tab, report, container]); | |||
|
|
|||
| return null; | |||
There was a problem hiding this comment.
Hard disagree that it is an established or clean pattern to create components that never render anything. I have never seen that before. I have asked other people, and they have never seen that before. Happy to discuss.
|
|
||
| const ReportTab: FC<{ children: (tab: any) => React.ReactNode; filters: any; report: ReportConfig }> = ({ | ||
| report, | ||
| const TabbedReportPanelComponent: FC<TabbedReportPanelProps> = ({ |
There was a problem hiding this comment.
Yes it is a style we have across many of our components, and the advantage is that you don't need to have code that destructures your props on multiple lines, you can condense it into a single line.
labkey-ui-ehr/src/ParticipantHistory/TabbedReportPanel/OtherReportWrapper.tsx
Outdated
Show resolved
Hide resolved
labkey-ui-ehr/src/ParticipantHistory/TabbedReportPanel/useReportTab.ts
Outdated
Show resolved
Hide resolved
| // The component should render the TabbedReportPanel with EHR.reports namespace | ||
| // This is verified indirectly by successful render | ||
| expect(screen.getByText('Loading reports...')).toBeVisible(); | ||
| await waitForReportsToLoad(); |
There was a problem hiding this comment.
Most of the tests in this file (34/55) only rely on await waitForReportsToLoad();, despite all of them having test names implying they're testing something specific, and some even having additional comments implying even more specific behavior is being tested. These two tests in particular (props passed to TabbedReportPanel) have the exact same test body, if you ignore the comments, I don't think that's particularly useful. We should be asserting something different for each test. The passes correct reportNamespace prop test in particular says the thing being tested is verified indirectly by successful render, but surely we could actually assert that the TabbedReportPanel is visible right?
Another example is the test case for readOnly mode, it's not asserting anything different than most of the tests in this file, but if you look at the implementation of the component we could easily assert that the SearchByIdPanel should not be visible, since we hide that in readOnly mode.
The tests in this file (and everywhere else) need to actually assert something. I don't think it is a useful test if the only thing being asserted is that the text "General" has appeared on the page somewhere. The coverage numbers don't matter if you aren't asserting anything in your tests.
If you can't actually assert something in the body of a test, then the test is probably worthless; maybe unit tests aren't appropriate for the particular test case.
There was a problem hiding this comment.
good call. I updated these tests.
| } | ||
|
|
||
| .update-report-button { | ||
| padding: 10px 20px; |
There was a problem hiding this comment.
Something to consider: these button styles don't match any other button styles within LabKey. I think they look good, but it's odd that we have completely different styles for these buttons. I would consider using the bootstrap styles that align with what you're doing e.g. btn btn-primary, btn btn-default, etc.
There was a problem hiding this comment.
Yeah I have to think about handling styling here. Some of that is linked to using @labkey/components or not.
There was a problem hiding this comment.
Well, because this isn't a full page app, you are getting the boostrap styles from @labkey/theme, so you should have full access to all of the normal bootstrap styles, but yes, you won't have access to the @labkey/components styles.
You can still override the styles in your own scss files here, but you will have to override them in a way that is compatible with the styles already being on the page (vs overriding them via the overrides.scss files), which means sometimes you will have to create really specific styles (this is why I recommend following BEM naming conventions), and in some rare cases you may have to do things like !important.
There was a problem hiding this comment.
These are all converted to BEM naming conventions.
|
@labkey-alan Alright tests, comments and css has been worked over. Let me know if you have any other feedback. Thanks. |
labkey-bpatel
left a comment
labkey-bpatel
left a comment
There was a problem hiding this comment.
Reviewed non-react files. Looks good
labkey-alan
left a comment
labkey-alan
left a comment
There was a problem hiding this comment.
There are still a lot of problems with the tests, especially the ParticipantReports tests
- Some tests are definitely not testing what they imply they're testing
- Some tests aren't testing anything useful
- Others are duplicates.
- Some assertions aren't thorough enough, leaving gaps for bugs that could be missed
- There are a bunch of mocks that don't seem to be necessary
- There are tests emitting react act errors
- I've probably missed some other issues, because there are so many tests
You need to be manually verifying every test that is generated. This is the third time in a row that there are obvious problems with the tests. Claude is pretty clearly not capable of writing tests perfectly, you're going to have to intervene.
I did leave some other feedback, but it was all mostly pretty minor.
There was a problem hiding this comment.
Running this test suite results in several console.error messages being logged that look like:
Warning: An update to ParticipantReports inside a test was not wrapped in act(...).
This should be resolved before merging.
| WebPart: jest.fn().mockImplementation(() => ({ | ||
| render: jest.fn(), | ||
| })), | ||
| Filter: { |
There was a problem hiding this comment.
I don't understand why we're mocking the LABKEY.FIlter in this test suite. I don't think these mocks are used, or provide any value. If you comment this out and run the test suite the tests continue to pass.
|
|
||
| // Mock LABKEY.WebPart for OtherReportWrapper | ||
| // Mock LABKEY API for OtherReportWrapper and ParticipantReports | ||
| // Note: Query.selectRows is mocked via @labkey/api mock above |
There was a problem hiding this comment.
yes, Query.selectRows is mocked above, but given that the component now uses dependency injection to allow you to override the fetchReports functionality it no longer makes API calls when you pass jest mocks (which the tests below do). We can remove the mocked selectRows above.
| }); | ||
|
|
||
| // Keep Query.selectRows mock for backward compatibility | ||
| // (used by some specific test overrides) |
There was a problem hiding this comment.
I don't believe these comments are true, and I don't believe setting a mockImplementation of Query.selectRows is useful. I don't see any specific tests that override this, and if I comment out the mockImplementation the tests still pass.
| // Note: Query.selectRows is mocked via @labkey/api mock above | ||
| (global as any).LABKEY = { | ||
| ...(global as any).LABKEY, | ||
| WebPart: jest.fn().mockImplementation(() => ({ |
There was a problem hiding this comment.
This doesn't seem useful. If I comment it out the tests still pass, and I don't see us overriding it, or checking if it is called, in any tests below.
| @@ -0,0 +1,214 @@ | |||
| import React from 'react'; | |||
| import { render, waitFor } from '@testing-library/react'; | |||
| import { Filter } from '@labkey/api'; | |||
| disabled={isResolving} | ||
| onClick={handleUpdateReport} | ||
| > | ||
| {'Search By Ids'} |
There was a problem hiding this comment.
| {'Search By Ids'} | |
| Search By Ids |
| * }), | ||
| * }); | ||
| */ | ||
| export const createTestAPIWrapper = ( |
There was a problem hiding this comment.
This is the only export in the file and it's not used anywhere. I would just delete this file.
| export function useReportTab( | ||
| report: ReportConfig, | ||
| filters: ReportFilters | ||
| ): { tab: ExtReportTab | null; targetRef: React.RefObject<HTMLDivElement> } { |
There was a problem hiding this comment.
I would define this return type above as:
interface ReportTabState {
tab: ExtReportTab | null;
targetRef: React.RefObject<HTMLDivElement>;
}and then change the line to:
| ): { tab: ExtReportTab | null; targetRef: React.RefObject<HTMLDivElement> } { | |
| ): ReportTabState { |
Generally speaking, inline type definitions aren't a good idea. TypeScript has notoriously cryptic error messages, and inline types make that worse. If you define/name your types then the error messages can be easier to parse, and give you something easy to search for.
| All Animals | ||
| </button> | ||
| <button | ||
| className={classNames('search-by-id-panel__filter-button', 'search-by-id-panel__filter-button--alive-at-center', { |
There was a problem hiding this comment.
eslint is highlighting this line, and several similar ones in this file, as needing to be formatted. While you could just run the formatter to appease eslint I think the formatted code looks pretty bad. I think it would probably be better to define the className for this button (and similar buttons in this component) above e.g.:
const aliveAtCenterButtonClassName = classNames(
'search-by-id-panel__filter-button',
'search-by-id-panel__filter-button--alive-at-center',
{
'search-by-id-panel__filter-button--active': filterType === FILTER_TYPE_ALIVE_AT_CENTER,
'search-by-id-panel__filter-button--inactive': filterType !== FILTER_TYPE_ALIVE_AT_CENTER,
}
);And then the button looks like:
<button
className={aliveAtCenterButtonClassName}
disabled={!activeReportSupportsNonIdFilters}
onClick={() => handleFilterModeChange(FILTER_TYPE_ALIVE_AT_CENTER)}
title={
!activeReportSupportsNonIdFilters
? 'This filter type is not supported for this report'
: undefined
}
>
All Alive at Center
</button> 6 participants
Rationale
Implements the first phase of React conversion of Animal History. This is currently a hidden view until we get to MVP. Link in the ehrAdmin page. Participant history is available if you have the experimental feature on, you'll get a link at the top of the old participant view which comes up when you click on an Id.
Spec is in this PR
Changes